Skip to content

Conversation

@JordanYates
Copy link
Contributor

Implements a relatively simple mechanism for automatically generating memory regions based devicetree partitions.
For a partition to result in a memory region, it must add the linker-region property to the appropriate partition.
Only partitions under the chosen/zephyr,flash node result in memory regions, under the assumption that this points at the onboard ROM, and that memory regions for external flash do not make sense.
Regions are named based on the label of the partition.

The primary purpose of this PR is to enable placing constants at absolute addresses in a portable manner.

Naming for FOR_EACH_NESTED and linker-region is provisional pending feedback.
Currently this has only been integrated into the Cortex linker script and enabled on a small subset of nRF hardware.

Implements #31073

Jordan Yates added 5 commits January 3, 2021 12:21
Adds a macro that operates on the results of previous evaluations.
This is mostly useful for calculating the maximum or minimum of an
arbitrary list of integers at compile time.

An implementation in a similar style to `Z_FOR_EACH_IDX` was attempted,
however macros such as `COND_CODE_1` were not properly being expanded
inside nested usage of `UTIL_OBSTRUCT`.

Signed-off-by: Jordan Yates <[email protected]>
Add test cases for the `FOR_EACH_NESTED` function.

Signed-off-by: Jordan Yates <[email protected]>
Add a new property to the child binding of `fixed-partition` to signify
that a memory region in the linker should be generated for this
partition.

Signed-off-by: Jordan Yates <[email protected]>
Adds two public macros for creating memory regions from devicetree
partitions. The `linker-region` property is only considered on
partitions under `chosen/zephyr,flash` as this is assumed to be the only
flash driver for which memory regions make sense.

`DT_PARTITION_REGIONS` declares the memory regions for consumption by
ld. While macro limitations mean that region names are quoted and there
are no seperators between regions, ld appears to handle this fine.

`DT_PARTITION_REGIONS_START` gets the memory address of the region with
the lowest origin address. The FLASH region will be constrained to this
address.

Signed-off-by: Jordan Yates <[email protected]>
Insert macros for generating memory regions from flash partitions that
have set the `linker-region` property.

Signed-off-by: Jordan Yates <[email protected]>
@github-actions github-actions bot added area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: Documentation area: Samples Samples area: Tests Issues related to a particular existing or missing test labels Jan 3, 2021
Jordan Yates added 2 commits January 3, 2021 16:31
Demonstrate how to place variables at constants memory addresses defined
by devicetree partitions.

Signed-off-by: Jordan Yates <[email protected]>
Enable the `linker-region` property on the NVS partition for a subset of
nordic boards to validate CI behaviour.

Signed-off-by: Jordan Yates <[email protected]>
@mbolivar-nordic mbolivar-nordic added the dev-review To be discussed in dev-review meeting label Jan 11, 2021
@mbolivar-nordic
Copy link
Contributor

Added dev-review per Slack:

jyates  3:58 PM
I would like to start getting feedback on https://github.com/zephyrproject-rtos/zephyr/pull/31077.
It has been relevant to several discussions here recently and I would like to know if the general method is accepted before putting in the effort of updating non-cortex linker scripts.

@JordanYates
Copy link
Contributor Author

JordanYates commented Jan 14, 2021

As a cross-link for posterity, this is also an alternative to #29794 for #27471

@ioannisg
Copy link
Member

As a cross-link for posterity, this is also an alternative to #29794 for #27471

I think this could be treated a complement to #27471, the current PR does not only deal with the sizing and placing of the code partition.

@JordanYates
Copy link
Contributor Author

I think this could be treated a complement to #27471, the current PR does not only deal with the sizing and placing of the code partition.

Could you clarify please? I'm not sure how to read that statement. Did you mean #29794?
Or are you saying that your PR solves problems additional to #27471?

@ioannisg
Copy link
Member

I think this could be treated a complement to #27471, the current PR does not only deal with the sizing and placing of the code partition.

Could you clarify please? I'm not sure how to read that statement. Did you mean #29794?
Or are you saying that your PR solves problems additional to #27471?

Yeap, sorry, I meant the PR, #29794 (not necessarily the exact solution presented there, but rather its generic idea, i.e. using the existing DT/Kconfig infrastructure to limit the code partition to a certain area)

@JordanYates
Copy link
Contributor Author

i.e. using the existing DT/Kconfig infrastructure to limit the code partition to a certain area)

Sure, this PR makes no statements about the code partition beyond shrinking the default ROM_SIZE so that it doesn't overlay defined linker-region partitions.

@galak
Copy link
Contributor

galak commented Jan 14, 2021

@JordanYates what's the implication if you assumed all partitions had linker-region property set?

@ioannisg
Copy link
Member

A limitation that I spot in the current implementation is that all the flash area beyond the storage_partition (or, rather, the first partition with the linker-region property) cannot be used by the Zephyr for the application image. This is problematic for all the boards that define the storage partition right after the boot partition. There has to be found a way to by pass this limitation (alternatively all the boards that not define storage_partition at the end will need to re-adjust their existing flash layout)

@JordanYates
Copy link
Contributor Author

@JordanYates what's the implication if you assumed all partitions had linker-region property set?

If by all partitions you mean the entire flash space is covered by linker-region, then the implication is that you have reserved all of your flash for custom uses and have no space left for an application.

@JordanYates
Copy link
Contributor Author

JordanYates commented Jan 14, 2021

A limitation that I spot in the current implementation is that all the flash area beyond the storage_partition (or, rather, the first partition with the linker-region property) cannot be used by the Zephyr for the application image. This is problematic for all the boards that define the storage partition right after the boot partition. There has to be found a way to by pass this limitation (alternatively all the boards that not define storage_partition at the end will need to re-adjust their existing flash layout)

Without changes to this PR, the resolution is simply to not put linker-region on those storage partitions that exist at the start of ROM. #27471 is not a problem for those boards currently as they must be manually setting CONFIG_FLASH_LOAD_OFFSET to work. Therefore it is already impossible for the partition to be overrun.

The special cases/behavior are starting to pile up however, and the solution no longer seems as clean as it did originally.
Having considered this problem in the light of your PR, I believe the core problem is the implicit behavior of the FLASH region when building standalone applications. Because the user is not explicitly saying where the code should live, we are left to make assumptions (currently "use it all!"), which will always be incorrect in odd cases. IMO the only watertight solution is to force the user to make this information explicit in some way. Once the FLASH region is using a defined memory space, my desired functionality can be implemented entirely by the linker snippet system.

Your single-image partition is one method of doing this. An alternative that wouldn't require touching existing board files would be to define FLASH as the total space occupied by boot_partition,slot0_partition,slot1_partition and scratch_partition, with the caveat that they couldn't have other partitions inserted between them.

@ioannisg
Copy link
Member

A limitation that I spot in the current implementation is that all the flash area beyond the storage_partition (or, rather, the first partition with the linker-region property) cannot be used by the Zephyr for the application image. This is problematic for all the boards that define the storage partition right after the boot partition. There has to be found a way to by pass this limitation (alternatively all the boards that not define storage_partition at the end will need to re-adjust their existing flash layout)

Without changes to this PR, the resolution is simply to not put linker-region on those storage partitions that exist at the start of ROM.

I believe, this is not ideal, it breaks uniformity across storage partition definitions in the various boards. I would not recommend this way forward; we should be trying to introduce common rules and conventions across definitions.

#27471 is not a problem for those boards currently as they must be manually setting CONFIG_FLASH_LOAD_OFFSET to work. Therefore it is already impossible for the partition to be overrun.

Unfortunately, this does not always hold - I am sure many they are not (so we're just lucky the samples and tests they run fit in the area preceding the storage partition, or the storage partition is not used in in-tree samples).

The special cases/behavior are starting to pile up however, and the solution no longer seems as clean as it did originally.
Having considered this problem in the light of your PR, I believe the core problem is the implicit behavior of the FLASH region when building standalone applications. Because the user is not explicitly saying where the code should live, we are left to make assumptions (currently "use it all!"), which will always be incorrect in odd cases.

IMO the only watertight solution is to force the user to make this information explicit in some way. Once the FLASH region is using a defined memory space, my desired functionality can be implemented entirely by the linker snippet system.

Agreed. And that's why I said initially that #31077 is rather a complement for #29794, not an alternative. We need to make it explicit which area can be used by the image code in all cases - not only when we build with boot loader.

Your single-image partition is one method of doing this.

The implementation #29794, although it does do the job, as you point out, it is not accepted by @galak @mbolivar-nordic, so we have to look for an alternative.

An alternative that wouldn't require touching existing board files would be to define FLASH as the total space occupied by boot_partition,slot0_partition,slot1_partition and scratch_partition, with the caveat that they couldn't have other partitions inserted between them.

So you suggest to "do some math" at Kconfig and/or linker stage :) It could theoretically be an alternative. I don't get why this would be a better solution, though.

To clarify, my "problem" here is that the logic to decide where to place and how to size the code partition (and other partitions) is split in 3 Zephyr main "components":

  • DeviceTree: where partitions are initially described, in the boards's dts files,
  • Kconfig: where chosen symbols become enabled, based on Kconfig input not available to DT (IS BOOTLOADER, IS chain-loadable image, etc.) and other Kconfig symbols are initialized (e.g. FLASH_LOAD_OFFSET/SIZE, ROM_START_OFFSET, etc.)
  • Linker script (where logic is used to further manipulate regions an entries, based, again on DT and/or Kconfig input). Your linker: generate memory regions from devicetree partitions #31077 is actually injecting additional logic at the linker.)

IMHO this is far from ideal. The solution is hard to follow and developers have a hard time to learn what is going on.

My attempt in #29794 was to move as much as we can at the DeviceTree stage, basically, "doing all the math we need" at DeviceTree partition layout descriptions, and then let Kconfig select the right layouts/partitions and initialize the proper symbols, so the Linker would not have to do anything (just accept the incoming configuration without further "internal" logic to derive region boundaries)

If we keep doing "partitioning" calculations at Kconfig/Linker stage, I would argue: why do we, then, need to define partitions in DeviceTree, at all? At Kconfig stage we already know the flash size and start address, so we could even do all the partition layout calculationss there, and remove all stuff from DeviceTree.

@JordanYates
Copy link
Contributor Author

It could theoretically be an alternative. I don't get why this would be a better solution, though.

It's not a better long term solution, but I think it would be a less invasive short term one. It doesn't change the current split of information across across devicetree/kconfig/linker, so it doesn't solve your problem, but it does make the error condition in #27471 impossible. Potentially that makes it a more acceptable solution given the dev-review comments around post-LTS.

The behavior change would be:

  • For applications that don't specify FLASH_LOAD_OFFSET, the linker script finds the lowest address from the listed partitions to use
  • For applications that don't specify FLASH_LOAD_SIZE, the linker script finds the highest address from the listed partitions and subtracts the value from the first step.

The main advantage is that user .dts files would not need to change in most cases.

If we keep doing "partitioning" calculations at Kconfig/Linker stage, I would argue: why do we, then, need to define partitions in DeviceTree, at all? At Kconfig stage we already know the flash size and start address, so we could even do all the partition layout calculationss there, and remove all stuff from DeviceTree.

I think the primary motivating reason is the flash_map API. When the partitions are in devicetree, you can iterate over the compatible using the C macros to instantiate each partition as a flash_area. This functionality is lost if partitions are defined in Kconfig.

@Laczen
Copy link
Contributor

Laczen commented Jan 15, 2021

@ioannisg, @JordanYates, let us set the goal of removing everything from Kconfig, and just set everything in the dts:
a. We can't put different partition definitions in one file, so different overlays are needed.
b. We don't want any DT_MCUBOOT, DT_IS_MCUBOOT, ... this means we need to get rid of the mcuboot header in the compilation: just define a partition for the header (the start of this header partition should be used for imagetool), the imagepartition itself is used for the compilation.

@galak
Copy link
Contributor

galak commented Jan 20, 2021

If by all partitions you mean the entire flash space is covered by linker-region, then the implication is that you have reserved all of your flash for custom uses and have no space left for an application.

I meant if you generated for every partition regardless of linker-region being set. I'm trying to see if we can remove the property, have the generation always occur, and its just based on use/application that a given region ends up coming into play with the linker.

@JordanYates
Copy link
Contributor Author

JordanYates commented Jan 21, 2021

I meant if you generated for every partition regardless of linker-region being set. I'm trying to see if we can remove the property, have the generation always occur, and its just based on use/application that a given region ends up coming into play with the linker.

So this works, even with overlapping memory regions, as long as you don't have multiple variables attempting to be placed at the same address. For example:

    FLASH                 (rx) : ORIGIN = ROM_ADDR, LENGTH = ROM_SIZE
    BOOTLOADER            (rx) : ORIGIN = 0K, LENGTH = 10K
    IMAGE_0               (rx) : ORIGIN = 10K, LENGTH = 100K
    IMAGE_1               (rx) : ORIGIN = 110K, LENGTH = 100K
    IMAGE_SWAP            (rx) : ORIGIN = 210K, LENGTH = 10K

Trying to place a variable in BOOTLOADER results in an error (overlaps with the application in FLASH)
While placing the variable in IMAGE_SWAP works, even though it overlaps the FLASH region (although not the data in FLASH).

[2/6] Linking C executable zephyr\zephyr_prebuilt.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       81148 B         1 MB      7.74%
      BOOTLOADER:          0 GB        10 KB      0.00%
         IMAGE_0:          0 GB       100 KB      0.00%
         IMAGE_1:          0 GB       100 KB      0.00%
      IMAGE_SWAP:           4 B        10 KB      0.04%
            SRAM:       16016 B       256 KB      6.11%
        IDT_LIST:         104 B         2 KB      5.08%
[6/6] Linking C executable zephyr\zephyr.elf

This would not be a solution to the problem without further changes however. The storage partition for example never has any variables in it, so there would still be no errors as FLASH encroaches into it. The linker output is also rather misleading as it implies that there is more flash available than there actually is. The region that the application is put into needs to accurately reflect the space available to it.

@JordanYates
Copy link
Contributor Author

JordanYates commented Jan 21, 2021

If we are willing to live with the misleading linker output, we could:

  1. Generate linker regions for all partitions under zephyr,flash
  2. Add an overlapping partition for standalone code as in Zephyr: re-work DT flash partitioning for Nordic nRF boards in the tree #29794
  3. Add a new Kconfig symbol CODE_PARTITION, which would default to 'standalone', with alternate defaults when CONFIG_MCUBOOT or other symbols
  4. Remove the zephyr,code-partition chosen node, DT_USE_CODE_PARTITION, FLASH_LOAD_OFFSET, FLASH_LOAD_SIZE, etc
  5. Add an alias to map FLASH to the region we actually want in the linker file REGION_ALIAS(FLASH CONFIG_CODE_PARTITION);

This approach has much to recommend it IMO. Flash sizes for standalone applications become explicit. A significant chunk of the devicetree Kconfig interaction is reduced to a single symbol with minimal meaning (what partition does the application go into).

@Laczen
Copy link
Contributor

Laczen commented Jan 21, 2021

@JordanYates, and what if we would generate linker sections only for partitions under the selected code-partition.

Flash sizes would become explicit for standalone and bootloader applications,
Sections can be used in bootloader applications as well,
Add the possibility for the image to access its version (by defining a header section in the partition),
Add the possibility to reserve space for the image trailer (by defining a trailer section in the code partition),
Defining a (sub)partition with label "image" could identify where the image should go. So the REGION_ALIAS() could map FLASH to "image".
No need to have any Kconfig symbols, just use the code-partition.

For standalone applications this would fallback to the partitions defined under flash0 (although IMO it would be better to redefine the flash layout in this case).

@JordanYates
Copy link
Contributor Author

what if we would generate linker sections only for partitions under the selected code-partition

I'm not sure what you mean here, it is memory regions we need to generate, not sections.
Additionally, code-partition points at a partition, not a collection of partitions. I am also proposing to do away with that chosen node altogether.

Defining a (sub)partition with label "image" could identify where the image should go. So the REGION_ALIAS() could map FLASH to "image".

This would break the ability for the same application to compile as standalone or chain-loaded. The purpose of the Kconfig symbol is to allow CONFIG_MCUBOOT to continue 'just working'.

@Laczen
Copy link
Contributor

Laczen commented Jan 21, 2021

what if we would generate linker sections only for partitions under the selected code-partition

I'm not sure what you mean here, it is memory regions we need to generate, not sections.
Semantics, read as memory regions.
Additionally, code-partition points at a partition, not a collection of partitions. I am also proposing to do away with that chosen node altogether.

If the partition it points to has subpartitions it can also point to a collection of partitions.

Defining a (sub)partition with label "image" could identify where the image should go. So the REGION_ALIAS() could map FLASH to "image".

This would break the ability for the same application to compile as standalone or chain-loaded. The purpose of the Kconfig symbol is to allow CONFIG_MCUBOOT to continue 'just working'.

The proposal would not break this ability. When there are no subpartitions everything would remain as is and CONFIG_MCUBOOT would continue to just work. However it would be possible to use an overlay with a more detailed split up and remove the usage of CONFIG_MCUBOOT (and avoid having to change Kconfig.zephyr for other bootloaders). Depending on whether a subpartition with the label "image" is defined the REGION_ALIAS() could either point to the complete partition or the subpartition with label "image".

A fit for all applications could be defined in one dts-file:
\flash0\image
\flash0\image\bootloader
\flash0\image\bootloader\image
\flash0\image\image0
\flash0\image\image0\image
\flash0\image\image0\image\image-hdr
\flash0\image\image0\image\image
...
\flash0\storage

compiling for code-partition = &flash0 would generate memory regions image and storage and maps FLASH to image,
compiling for code-partition = &bootloader would generate memory region /image/bootloader and map FLASH to image/bootloader/image,
compiling for code-partition = &image0 would generate memory regions /image/image0/image and map FLASH to image/image0/image.
compiling for code-partition = &image0/image would generate ... and map FLASH to image/image0/image/image.

The partitioning could probably be simplified if the is some rule like: "if there is no subpartition labeled "image" then map the flash to the complete partition".

@JordanYates
Copy link
Contributor Author

From what I can tell, you are advocating for the solution where the decision to use a bootloader or not happens before either devicetree or Kconfig have run, either in CMake or from the command line (There is no other way to swap around which partition has the "image" label AFAICT).

That approach allows Kconfig symbols to be dropped at the cost of adding another configuration layer to the build system.
Personally I think a single Kconfig symbol that selects which partition to use is a small price to pay for not complicating the build system further.

@Laczen
Copy link
Contributor

Laczen commented Jan 21, 2021

From what I can tell, you are advocating for the solution where the decision to use a bootloader or not happens before either devicetree or Kconfig have run, either in CMake or from the command line (There is no other way to swap around which partition has the "image" label AFAICT).

That approach allows Kconfig symbols to be dropped at the cost of adding another configuration layer to the build system.
Personally I think a single Kconfig symbol that selects which partition to use is a small price to pay for not complicating the build system further.

No, no, I am not advocating for another solution. The only selection would happen by the setting of code-partition (as it already is). This would generate a linker region "image" from its subpartitions (not subsubpartitions) and this would be where the image should go.

OK, it will not be so straightforward because you cannot have multiple partitions with the same label.

But by turning the idea around a bit it might even be better: by selecting code-partition the region where the image should live is defined and this generates linker memory regions from the parent partition (if there is a parent).

@ioannisg
Copy link
Member

ioannisg commented Jan 21, 2021

That approach allows Kconfig symbols to be dropped at the cost of adding another configuration layer to the build system.
Personally I think a single Kconfig symbol that selects which partition to use is a small price to pay for not complicating the build system further.

I guess that was what was presented in #29794 (if i understood the comment correctly)

@Laczen
Copy link
Contributor

Laczen commented Jan 21, 2021

@JordanYates, for some reason your name does not easily pop-up on github. I created a new issue #31487 for image generation. This could be extended with linker memory regions.

@JordanYates
Copy link
Contributor Author

I guess that was what was presented in #29794 (if i understood the comment correctly)

Not quite, your proposal adds one symbol, I am proposing deleting all current symbols except for one.
The current FLASH_LOAD_OFFSET, FLASH_LOAD_SIZE and USE_DT_CODE_PARTITION symbols can all be removed if the application must be placed into a partition. If users wish to modify where the image is placed, that becomes a devicetree overlay for that application, instead of overriding Kconfig symbol defaults.

I think I will mock up this solution on a few boards just to see if there are unforeseen problems.

@galak galak removed the dev-review To be discussed in dev-review meeting label Mar 11, 2021
@JordanYates
Copy link
Contributor Author

Superseded by #33656

@JordanYates JordanYates deleted the 210102_linker_sections branch August 15, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Base OS Base OS Library (lib/os) area: Boards area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Documentation area: Samples Samples area: Tests Issues related to a particular existing or missing test area: Testsuite Testsuite platform: nRF Nordic nRFx

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants